-
Notifications
You must be signed in to change notification settings - Fork 940
Add business metrics support for STS, SSO and Profile credential providers #6426
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add business metrics support for STS, SSO and Profile credential providers #6426
Conversation
077e209
to
685e7ac
Compare
.../src/main/java/software/amazon/awssdk/auth/credentials/internal/ProfileCredentialsUtils.java
Outdated
Show resolved
Hide resolved
...ain/java/software/amazon/awssdk/auth/credentials/ChildProfileCredentialsProviderFactory.java
Outdated
Show resolved
Hide resolved
services/sso/src/main/java/software/amazon/awssdk/services/sso/auth/SsoCredentialsProvider.java
Outdated
Show resolved
Hide resolved
...a/software/amazon/awssdk/services/sts/internal/AssumeRoleWithWebIdentityRequestSupplier.java
Outdated
Show resolved
Hide resolved
...src/main/java/software/amazon/awssdk/services/sts/auth/StsAssumeRoleCredentialsProvider.java
Show resolved
Hide resolved
...ava/software/amazon/awssdk/services/sts/auth/StsWebIdentityTokenFileCredentialsProvider.java
Show resolved
Hide resolved
...ts/src/it/java/software/amazon/awssdk/auth/source/ProfileAssumeRoleFailureUserAgentTest.java
Outdated
Show resolved
Hide resolved
.../src/main/java/software/amazon/awssdk/auth/credentials/internal/ProfileCredentialsUtils.java
Outdated
Show resolved
Hide resolved
return stsCredentialsProviderFactory().create(sourceCredentialsProvider, profile); | ||
String sourceMetrics = extractBusinessMetricsFromProvider(sourceCredentialsProvider); | ||
|
||
String source = BusinessMetricFeatureId.CREDENTIALS_PROFILE_SOURCE_PROFILE.value(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A nit on "source" naming - at a quick glance, I might read that as being the source credentials rather than just a feature ID - I know its longer, but maybe sourceFeatureId
?
Additionally - it looks like the logic here from 264-273 is duplicated below on line 284, does it make sense to refactor to a method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handled the duplicate logic, Agree with sourceFeatureId
naming. Need to make the changes to all the files, will make the changes at last after team review, incase if anyone have anything different.
.../src/main/java/software/amazon/awssdk/auth/credentials/internal/ProfileCredentialsUtils.java
Show resolved
Hide resolved
.../java/software/amazon/awssdk/services/sts/auth/StsAssumeRoleWithSamlCredentialsProvider.java
Show resolved
Hide resolved
...ava/software/amazon/awssdk/services/sts/auth/StsWebIdentityTokenFileCredentialsProvider.java
Show resolved
Hide resolved
Address PR feedback Adding Unit tests Apply PR feedback to previous PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this standalone test and consolidated it into StsCredentialsProviderUserAgentTest.java as a parameterized test.
|
36324ae
into
feature/master/feature-ids-implementation
This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one. |
Adds business metric tracking for credentials
Motivation and Context
Keeping track of how users are providing credentials to SDKs and which credentials providers are being used.
Modifications
This PR adds business metrics support for these credential providers:
Key Technical Changes
Source Propagation: Introduces
source
parameter on credential provider builders to track credential provider chainsm/n,g,i
(profile + env vars + assume role)Provider Name Updates: Changes existing
providerName()
methods to return business metric codes instead of full class namesChain Tracking: Supports credential scenarios like (In
ProfileCredentialsUtils
)credential_source
,source_profile
, web identity token , SSO session,credentials_process
Example User-Agent Outputs
m/g
m/n,o,i
m/g
(only successful provider shown)Deviations from specs:
Not using
SSO legacy
("u") because it doesn't make sense - It's already tracked as legacy via theCREDENTIALS_PROFILE_SSO_LEGACY
("t") value. Once in the SSO provider legacy doesn't really matter. The regularCREDENTIALS_PROFILE_SSO
value of ("s") can be used for both cases. You can't really set a legacy client without coming from the profileTesting
Added unit tests for these
CREDENTIALS_STS_ASSUME_ROLE("i") - StsAssumeRoleCredentialsProvider
CREDENTIALS_STS_ASSUME_ROLE_SAML("j") - StsAssumeRoleWithSamlCredentialsProvider
CREDENTIALS_STS_ASSUME_ROLE_WEB_ID("k") - StsAssumeRoleWithWebIdentityCredentialsProvider
CREDENTIALS_STS_FEDERATION_TOKEN("l") - StsGetFederationTokenCredentialsProvider
CREDENTIALS_STS_SESSION_TOKEN("m") - StsGetSessionTokenCredentialsProvider
CREDENTIALS_PROFILE("n")- ProfileCredentialsProvider
Performed integ test for these cases
Scenarios like: "p,0,i" and "o,n,i" for assume role with credential source and source profile, "r,s" for modern SSO session configuration, and "q,k" for profile-based web identity token flows. Legacy SSO testing with "t,s" business metrics is pending completion.
Screenshots (if appropriate)
Types of changes
Checklist
mvn install
succeedsscripts/new-change
script and following the instructions. Commit the new file created by the script in.changes/next-release
with your changes.License